Birmingham | ITP-Jan | Ahmad Roman Sanaye | Sprint 3 | Practice-tdd#1167
Birmingham | ITP-Jan | Ahmad Roman Sanaye | Sprint 3 | Practice-tdd#1167RomanSanaye wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
…; pass all test cases
cjyuan
left a comment
There was a problem hiding this comment.
I noticed some minor format inconsistency.
Have you installed the prettier VSCode extension and enabled "Format on save/paste" on VSCode, as recommended in
https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/blob/main/readme.md
?
If you have enabled "Format on save" but it is not working, it is likely that you haven't assign a formatter for JS file. This could happen if you have zero or multiple extensions that can format .js file.
If you have installed "Prettier" extension. To assign it as the formatter of JS code, you can try:
- Use "Format document" to format the JS file. Sometimes, VSCode will ask you to choose a formatter, and you can manually select "Prettier".
- Edit
settings.jsonand set Prettier as the default formatter for JS.
See: https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode
Sprint-3/2-practice-tdd/count.js
Outdated
| if (numberOfChar === 0) { | ||
| return `0 ${findCharacter} found.` | ||
| } |
There was a problem hiding this comment.
The function is expected to return an integer number. Returning a string would make the function difficult to use because the caller would need to check the data type of the return value.
There was a problem hiding this comment.
Hello @cjyuan ,
I have updated the function so it now always returns an integer. Returning 0 instead of a string ensures the function is easier to use and matches the expected return type.
| expect(getOrdinalNumber(33)).toEqual("33rd"); | ||
| expect(getOrdinalNumber(333)).toEqual("333rd"); | ||
| }); | ||
| test("should append 'th' for numbers ending with 4 and more than 4", () => { |
There was a problem hiding this comment.
... ending with 4-9, 0 might be more concise.
There was a problem hiding this comment.
I have updated the ordinal number test descriptions to group numbers ending in 4-9 and 0, making the tests more concise while still covering all cases.
| // When the repeatStr function is called with these inputs, | ||
| // Then it should return the original `str` without repetition. | ||
|
|
||
| test("should print the original string", () => { |
There was a problem hiding this comment.
"print" may suggest the function should output the string.
There was a problem hiding this comment.
I have updated the test description to say “return” instead of “print” to accurately reflect that the function returns a string rather than printing it.
| // When the repeatStr function is called with these inputs, | ||
| // Then it should return an empty string. | ||
|
|
||
| test("should return an empty string", () => { |
There was a problem hiding this comment.
You could probably make this description more informative by including the when ... part.
There was a problem hiding this comment.
I have revised the test descriptions to include the input condition (when …) so they clearly state under what circumstances the function should return the expected result.
|
There is no new commits on this branch, and the comments are not yet addressed. If you have responded to the comments, you may want to check if the responses are in "Pending" state. Other users cannot see the pending comments. |
| function countChar(stringOfCharacters, findCharacter) { | ||
| return 5 | ||
| if (!stringOfCharacters || !findCharacter) { | ||
| return 0; | ||
| } | ||
| let numberOfChar = 0; | ||
| for (let wantedChar of stringOfCharacters) { | ||
| if (wantedChar === findCharacter) { | ||
| numberOfChar++; | ||
| } | ||
| if (numberOfChar === 0) { | ||
| return 0; | ||
| } | ||
| } | ||
| return numberOfChar; | ||
| } |
There was a problem hiding this comment.
This function has a bug; it does not always return the correct count.
If you cannot figure out the bug, try testing a few more samples in count.test.js.
|
Hi @cjyuan , thanks for your feedback. I have addressed all the comments:
I have also added the “Needs Review” label. Please let me know if anything else is required. |
|
Changes look good, and thanks for the clear explanation in the responses. Well done. |
Self checklist
Changelist
Implemented the following functions:
Each function handles the required cases and passes all Jest test cases, including edge cases (positive, zero, and negative values where applicable).
All tests are passing successfully.